fix(gc): release the prototype-registry latch when a prune drains it (#7737) - #7740
Conversation
…7737) The latch was one-way, so a single Object.setPrototypeOf anywhere in a process permanently disabled #7733's per-evacuated-object fast path. The set moves under the mutex so the clear cannot race an in-flight insert. Claude-Session: https://claude.ai/code/session_01Y1QZ5wUP9gRSwpiweT4Wix
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe runtime now updates the prototype registry latch under its mutex and clears it after dead-owner pruning drains the registry. A regression test verifies latch release. Project version references and the changelog were updated. ChangesPrototype registry latch
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Cargo.toml`:
- Line 318: Revert both release metadata changes: restore
[workspace.package].version in Cargo.toml at lines 318-318 to 0.5.1431, and
restore CLAUDE.md’s Current Version at lines 11-11 to 0.5.1431. Leave the
changelog fragment as the source for this contributor PR’s runtime fix.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: bddc510c-8cec-404e-a1b3-25ce4d73df23
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (4)
CLAUDE.mdCargo.tomlchangelog.d/7739-prototype-latch-drain.mdcrates/perry-runtime/src/object/prototype_chain.rs
|
|
||
| [workspace.package] | ||
| version = "0.5.1431" | ||
| version = "0.5.1432" |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Keep release version metadata out of this contributor PR.
The PR-keyed changelog fragment is the correct source for this runtime fix. Revert both version metadata edits; maintainers apply them during merge or release.
Cargo.toml#L318-L318: restore[workspace.package].versionto0.5.1431.CLAUDE.md#L11-L11: restoreCurrent Versionto0.5.1431.
As per coding guidelines, external contributor PRs must not change [workspace.package].version. Based on learnings, contributors must not update release metadata when a changelog.d/ fragment is present.
📍 Affects 2 files
Cargo.toml#L318-L318(this comment)CLAUDE.md#L11-L11
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Cargo.toml` at line 318, Revert both release metadata changes: restore
[workspace.package].version in Cargo.toml at lines 318-318 to 0.5.1431, and
restore CLAUDE.md’s Current Version at lines 11-11 to 0.5.1431. Leave the
changelog fragment as the source for this contributor PR’s runtime fix.
Sources: Coding guidelines, Learnings
Fixed
The prototype registry's fast-path latch was one-way, so a single
Object.setPrototypeOfdisabled it for the life of the process (GC follow-ups from #7733: one-way prototype latch (#7510 shape), missing positive-direction pacing test, unmeasured grow-then-churn transition, advisory-only ratchet #7737 item 1).OBJECT_PROTOTYPES_NONEMPTYis set when a non-meta-capable owner records a prototype and was never cleared — including byprune_dead_object_prototype_ownerswhen itsretain()drains the map back to empty.That became load-bearing with perf(gc): stop re-marking a heap where nothing dies — yield-adaptive major pacing + the evacuation move-hook mutex #7733, which added a third reader: the evacuation move hook
object_static_prototype_owner_movedconsults the latch once per moved object to skip a process-globalMutex<HashMap>and a SipHash lookup (2.5 M of each onretain.ts). So one incidental re-prototyping early in a run — of aRegExp, say — silently forfeited that win for every subsequent evacuation, with no signal that it had happened.This is perf(gc): layout side tables are 34% of object construction — the construction/death half of #5094 (allocation is 7.7%) #7510's finding recurring: "one immortal side-table entry nullified every
is_empty()fast path", now with a third victim.The clear could not simply be added, and the reason is the interesting part. The latch was stored outside the mutex, deliberately before the insert, so that a reader observing it never misses a committed entry. Clearing under the lock against a set outside it loses entries:
true;— leaving a non-empty map with the latch false, which every reader skips. The set therefore moves under the same mutex, still before the insert, which serialises (1) and (3) against (2) and makes the interleaving impossible. The publish property is unchanged: a reader that sees
truetakes the lock and so sees whatever the writer committed. No extra cost — that path acquired the lock on the next line anyway.The regression test's load-bearing assertion is the last one, that the latch comes back down; everything before it passes with the bug present. Verified by removing the clear: "the registry is empty but the latch is still armed, so every evacuated object keeps paying the mutex + SipHash lookup for the rest of the process".
Summary by CodeRabbit
Bug Fixes
Documentation
Chores